THRIFT-6011: Make compiled Go code formatting compatible with gofmt#3493
Merged
Conversation
mergeable
Bot
added
compiler
github_actions
kpumuk
changed the title
Jens-G
approved these changes
May 18, 2026
fishy
approved these changes
May 19, 2026
| run: gofmt -s -d -e $(git ls-files | grep "\.go$") | ||
| run: | | ||
| git ls-files '*.go' > /tmp/go-files.txt | ||
| xargs -r gofmt -s -l -e < /tmp/go-files.txt > /tmp/gofmt-files.txt |
Member
There was a problem hiding this comment.
with xargs should we use >> over >? with >, if multiple go files have issues, we would only report (cat on the next line) issues from the last go file?
Member
Author
There was a problem hiding this comment.
No, not really. The redirection applies to the xargs command, not to each subprocess it starts. All output from the invoked gofmt processes is collected through that single stdout stream and written to /tmp/gofmt-files.txt.
Member
Author
There was a problem hiding this comment.
Ha, an example just came through https://github.com/apache/thrift/actions/runs/26098116881/job/76747159994?pr=3495
| find test/go/src/gen tutorial/go/gen-go -name '*.go' -type f -print | ||
| } > /tmp/generated-go-files.txt | ||
|
|
||
| xargs -r gofmt -l -e < /tmp/generated-go-files.txt > /tmp/generated-gofmt-files.txt |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This is a follow-up to THRIFT-5969, where @fishy mentioned that if I attempt to make Go compiler to produce
gofmt-compatible code, they will help me review it :-)The change actually makes Thrift compiler for Go to generate clean
gofmt-compatible code. There were a few low hanging fruits, such as spaces around curly braces, but the most challenging part is newlines between blocks of generated code: instead of simply printing new lines after it, we need to track what was generated before and insert new lines before the block so we don't produce excessive new line characters. Another tricky area is aligning struct fields, which was done by finding the lengths before rendering the struct.Additionally, imports have been sorted to separate standard library from thrift, which is compatible with more strict formatters.
Important
One notable change is how the generated package names are produced, where we would use the same technique as with variables to avoid package names that match keywords. For example, today package name
defaultwould not compile.The output was tested on all thrift files found in the repository. There are 210
.thriftfiles, with 18 failing the generation, generating 901 Go files (with nogofmtcomplains).Summary of the failing generation (pasted output to AI for summarization, sorry for this)
11 fail because Go cannot use slices/pointers as map keys
[]int32,[]int8,[]MyEnum3,[]*Pongtest/ThriftTest.thrift,test/Include.thrift,test/EnumTest.thrift,lib/php/test/Resources/ThriftTest.thrift, etc.5 fail because Rust recursive-test IDLs use include paths that are not resolvable from the naive repo-root generation command
CityServices.thrift,Vehicles.thrift,Buses.thrift,LightRail.thriftthrift -r file.thriftfrom repo root.1 intentionally invalid IDL
test/BrokenConstants.thrift98765432109876543211 more map-key failure in
skiptest_version_2.thrift[]*Pong.Comparison
Imports
Creates 3 distinct groups: stdlib, thrift library, local code.
Struct fields
Enums
func OperationFromString(s string) (Operation, error) { switch s { - case "ADD": return Operation_ADD, nil - case "SUBTRACT": return Operation_SUBTRACT, nil - case "MULTIPLY": return Operation_MULTIPLY, nil - case "DIVIDE": return Operation_DIVIDE, nil + case "ADD": + return Operation_ADD, nil + case "SUBTRACT": + return Operation_SUBTRACT, nil + case "MULTIPLY": + return Operation_MULTIPLY, nil + case "DIVIDE": + return Operation_DIVIDE, nil } return Operation(0), fmt.Errorf("not a valid Operation string") }Full Diff
Click to expand
[skip ci]anywhere in the commit message to free up build resources.